fix(voip): add .js suffix to WAProto import — fixes ERR_MODULE_NOT_FOUND on install#545
Conversation
…a NOT NULL Round-5 audit on develop @ 2155e25 (post-PR-#537 merge) flagged three follow-ups. All three applied here. ## SIG-P2-NOVO — `readVarint` off-by-one in the shift guard `src/Signal/libsignal.ts` The guard was `if (shift > 35)`, but JavaScript `<<` is modulo-32 on the right operand. The 5 valid byte shifts are 0, 7, 14, 21, 28; after the 5th byte the increment makes `shift = 35`. With `>`, byte 6 was still read on the next iteration and `(byte & 0x7f) << 35` silently folded into `<< 3`, producing a corrupt value that the outer `>>> 0` happily accepted as a "valid" uint32. The effect was subtle: a crafted PreKeyWhisperMessage with a 6-byte varint field tag could neutralise `extractIdentityFromPkmsg`'s field-3 (identityKey) parse — degrading the audit-SIG-A1 identity-change detection signal to undefined for that pkmsg. Decryption itself stays correct (libsignal processes the ciphertext independently); only the WARN-on-identity-change loop loses visibility. Fix: `shift >= 35` (one character). Comment rewritten to document the JS shift-mod-32 reasoning so future readers don't undo it. ## MDB-schema-raw_string — `jid.raw_string` NOT NULL `src/Utils/multi-db-sqlite/schemas/msgstore.ts` `raw_string TEXT` (nullable) had two distinct failure paths: (1) SQLite treats NULL as DISTINCT inside a UNIQUE index, so a malformed insert path producing NULL would silently bypass the `jid_raw_string_idx` uniqueness guarantee. (2) `selectJidIdByRaw` filters by `WHERE raw_string = ?`, and SQL `NULL = NULL` is FALSE — a row with raw_string NULL would never be findable again, making `rowIdFor` throw "failed to materialize jid row" for every subsequent access. `CREATE TABLE IF NOT EXISTS` is a no-op when the table exists, so legacy databases keep the nullable column (no migration risk); new databases get the constraint enforced. ## MDB-P2-A — `jid_map.sort_id` NOT NULL DEFAULT 0 `src/Utils/multi-db-sqlite/schemas/msgstore.ts` `sort_id INTEGER` (nullable) was first flagged in round-3. A row inserted without an explicit `sort_id` would end up as NULL. `ORDER BY sort_id DESC` ranks NULLs LAST in SQLite, which would silently demote a freshly-inserted-but-NULL row below older ones — the opposite of the `getAllLidsForPn` "last write wins" intent. `upsertMap` always provides a real epoch-ms tick today, so the default is purely defensive against future code paths that bypass upsertMap. ## Not applied (intentional) - **SOCK-P1** (`ev.flush()` incondicional) — toca buffer behavior that feeds the carousel/list/button render path. Per project constraint (no regressions in carousel/list/button), needs E2E coverage before any change. - **UTL-P1-A/B** (subarray aliasing + double-count fileLength) — needs benchmark/integration test before mutating HMAC stream and upload pipeline. - **MDB-P1-C** (`clear()` retry) — needs crash-recovery test. - **MSG-P1-A/B/C/D** — still deferred pending E2E, same as PR #537. ## Validation - `tsc --noEmit -p tsconfig.json` exit 0 - `eslint` on the 2 touched files: 0 errors - Local jest failures are pre-existing `better-sqlite3` native binding misses on Windows toolchain — unrelated. CI Linux is the gate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Code-review feedback on PR #538: 1. The three comments each opened with an audit-ID prefix (`(audit SIG-P2-NOVO)`, `audit MDB-schema-raw_string:`, `audit MDB-P2-A:`). These rot — a reader six months from now won't recognise the IDs without spelunking through PR history. Audit cross-references belong in the commit message and PR description; the in-tree comment should explain the WHY only. Dropped all three prefixes; the technical explanations stay. 2. `// SQL NULL = NULL is FALSE` was technically wrong. In SQL, `NULL = NULL` evaluates to UNKNOWN, not FALSE. The downstream effect in a `WHERE` clause is identical (row isn't returned), but the imprecise wording would make a SQL-savvy reader pause and question the rest of the comment. Rewritten to: `SQL NULL = NULL evaluates to UNKNOWN, so the row is never returned`. No functional change — pure documentation cleanup. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ESM strict (this package has `"type": "module"`) requires explicit file
extensions on relative imports. `src/Voip/signaling/bridge.ts:36` was the
ONLY file in the codebase importing `WAProto/index` without the `.js`
suffix — every other ~40 import call site already uses
`'../../WAProto/index.js'`.
The compiled output landed in `lib/Voip/signaling/bridge.js` as
`from '../../../WAProto/index'` (no `.js`). `tsc-esm-fix` patches
`.js` suffixes onto relative imports that stay inside `src/`, but
`../../../WAProto/index` reaches OUTSIDE the source tree and is left
alone. Result: a consumer installing this fork via
`npm install github:rsalcara/InfiniteAPI#develop` would crash at first
require of the VoIP bridge with:
Error [ERR_MODULE_NOT_FOUND]: Cannot find module
'/var/www/.../node_modules/baileys/WAProto/index'
imported from .../lib/Voip/signaling/bridge.js
Fix is one character: `'../../../WAProto/index'` →
`'../../../WAProto/index.js'`. Confirmed locally — the rebuilt
`lib/Voip/signaling/bridge.js` now emits the correct
`from '../../../WAProto/index.js'`.
Reproduces directly: deploy the broken build, start the consumer with
PM2, the VoIP module is touched during signaling init and the process
dies before the WhatsApp socket reaches `connection:open`. The earlier
`device_removed` 401 in the user's log is unrelated (regular session
logout on a removed device).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
There was a problem hiding this comment.
Pull request overview
This PR addresses an ESM runtime resolution failure in the VoIP signaling bridge by making the WAProto import explicitly reference index.js, and also includes small hardening changes in Signal varint parsing and the msgstore SQLite schema.
Changes:
- VoIP: fix Node ESM
ERR_MODULE_NOT_FOUNDby adding the.jssuffix to theWAProtoimport. - Signal: tighten
readVarintvalidation to reject overlong (6-byte) uint32 varints. - SQLite: strengthen
msgstore.dbschema withNOT NULL/DEFAULTconstraints to avoid NULL-driven uniqueness/sorting edge cases on newly created DBs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Voip/signaling/bridge.ts |
Adds .js suffix to the WAProto import to satisfy Node ESM resolution. |
src/Utils/multi-db-sqlite/schemas/msgstore.ts |
Tightens new-db constraints for jid.raw_string and jid_map.sort_id to prevent NULL-related failure modes. |
src/Signal/libsignal.ts |
Adjusts varint length guarding to avoid JS bitshift modulo-32 corruption on overlong varints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Reject once shift would reach 35 — NOT just exceed it. The 5 | ||
| // valid byte shifts are 0, 7, 14, 21, 28. After processing byte 4 | ||
| // (`shift` was 28), this increment makes `shift = 35`. With a | ||
| // `shift > 35` guard, byte 5 would still be read on the next loop | ||
| // iteration and `(byte & 0x7f) << 35` would silently fold into |
O bug
Quem instala o pacote via `npm install github:rsalcara/InfiniteAPI#develop` recebe:
```
Error [ERR_MODULE_NOT_FOUND]: Cannot find module
'/var/www/.../node_modules/baileys/WAProto/index'
imported from .../lib/Voip/signaling/bridge.js
```
Root cause
ESM strict (`"type": "module"` no package.json) exige extensão `.js` em imports relativos. `src/Voip/signaling/bridge.ts:36` é o único arquivo do projeto inteiro que importa `WAProto/index` sem o `.js` — outros ~40 imports do WAProto estão corretos.
`tsc-esm-fix` (que normalmente adiciona `.js` automaticamente) patches imports que ficam dentro de `src/`, mas `../../../WAProto/index` aponta pra fora da árvore source e fica intacto. Resultado: `lib/Voip/signaling/bridge.js` saiu sem `.js`, Node ESM falha ao resolver.
Fix
Um caractere:
```diff
```
Rebuild confirma — `lib/Voip/signaling/bridge.js` agora emite `from '../../../WAProto/index.js'` corretamente.
Test plan
🤖 Generated with Claude Code
Summary by cubic
Fixes an ESM crash in the VoIP bridge by adding the missing .js suffix to the
WAProtoimport. Also tightens varint parsing and SQLite schema constraints to prevent subtle parsing and ordering bugs.Bug Fixes
.jssuffix to../../../WAProto/index.jsto satisfy ESM resolution and preventERR_MODULE_NOT_FOUND.shift >= 35inreadVarintto reject 6-byte uint32 varints and preserve identity-key parsing.jid.raw_stringNOT NULL andjid_map.sort_idNOT NULL DEFAULT 0 to enforce uniqueness and correct ordering.Migration
Written for commit 81fa978. Summary will update on new commits.